-
Notifications
You must be signed in to change notification settings - Fork 634
Fix typedef binding with CJS exports=
#826
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
With this PR, top-level typedefs in file with commonjs exports (`module.exports=x` or `exports=x`) are no longer added as exports of the file. They're added as exports of whatever is `export=`. The big change is that, in Strada, non-top-level typedefs are also exported. But it never made sense for these two scoped type aliases to be exported, let alone *both*: ```js function one() { /** @typedef {string} T */ /** @type {T} */ var s = 's' } function two() { /** @typedef {number} T */ /** @type {T} */ var n = 1 } /* @type {T} */ var error = "I AM ERROR" ``` I'm not sure that I declared the local and export='d symbols in the right way, so I'd appreciate expert opinions there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates the typedef binding behavior in CommonJS modules so that only top-level typedefs are exported via the export= assignment, correcting unintended exports of scoped typedefs. Key changes include updating baseline error files to reflect the new behavior and modifying binder.go to delay binding of JSTypeAliasDeclarations when the file is a CommonJS module.
Reviewed Changes
Copilot reviewed 44 out of 55 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
testdata/baselines/reference/submodule/...errors.txt | Updated error messages and counts reflecting removal of extra export assignment issues |
internal/binder/binder.go | Introduced delayed binding for JSTypeAliasDeclarations and added delayedBindJSDocTypedefTag to bind typedefs conditionally |
Files not reviewed (11)
- testdata/baselines/reference/submodule/compiler/checkJsTypeDefNoUnusedLocalMarked.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/importingExportingTypes.symbols: Language not supported
- testdata/baselines/reference/submodule/conformance/importingExportingTypes.symbols.diff: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespace.symbols: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespace.symbols.diff: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespaceCjs.symbols: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportAliasExposedWithinNamespaceCjs.symbols.diff: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsImportTypeBundled.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsTypeAliases.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsTypedefAndLatebound.errors.txt: Language not supported
- testdata/baselines/reference/submodule/conformance/jsDeclarationsTypedefPropertyAndExportAssignment.types: Language not supported
if b.file.Symbol != nil { | ||
if exportEq := b.file.Symbol.Exports[ast.InternalSymbolNameExportEquals]; exportEq != nil && b.file.CommonJSModuleIndicator != nil { | ||
for _, node := range b.delayedTypeAliases { | ||
b.declareSymbol(ast.GetSymbolTable(&exportEq.Exports), exportEq /*parent*/, node, ast.SymbolFlagsTypeAlias, ast.SymbolFlagsTypeAliasExcludes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for one level of indirection to a local namespace-y thing, but what about an aliased namespacey thing? Eg
const mod = require("./mod.js")
/**
* @typedef {string} T
*/
module.exports = mod
or, abusing a ts construct to do it locally,
namespace ns {
export namespace inner {}
}
import mod = ns.inner
/**
* @typedef {string} T
*/
module.exports = mod
mod
's symbol will be an alias, and this will just patch the typedefs onto the alias symbol, and not the alias symbol's ultimate target, which, dollars to donuts, means it's going to effectively go missing without extra lookup logic in the checker, similar to what we used to have. 🥹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even simpler, this also reproduces the problem.
function f() { }
/** @typedef {string} T */
module.exports = f
internal/checker/checker.go
Outdated
if namespace == nil || ast.NodeIsMissing(right) { | ||
return nil | ||
var immediate *ast.Symbol | ||
alias := c.resolveEntityName(left, ast.SymbolFlagsAlias, true /*ignoreErrors*/, true /*dontResolveAlias*/, location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. So if the resolution of the LHS of a qualified name fails, we try to look it up with only one level of aliasing? This seems... weird? While this might work in some scenarios, if getExportsOfSymbol
doesn't return the merged in aliases, declaration emit is going to struggle to figure out how to name these typedefs, as traversing the exports is how symbols are named. Basically, we'd like to interpret
/**
* @typedef {number} Foo
*/
module.exports = name
as
export * from name // even though this isn't a thing in ESM (yet)
export type Foo = number
I think if we want to handle this in a non-jank way, we need to bind a module.exports = name
in a file with typedefs not as an alias but instead as a (new?) namespacey symbol kind that has logic in getExportsOfSymbol
to merge together the typedef set and the resolved, late-bound alias's set. (Once again, a callback to the .cjsSymbolMerged
logic we used to have)
I'm also honestly a lil unsure what a syntactically driven declaration emit for this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just add support for parsing/checking export * from name
in declaration files and actually do that transform on reparse? In theory it'll be added to esm alongside the module declarations proposal, probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this in 1cffb6e, but it results in just as many hacks, in the same places, as the previous commit. That's because export *
isn't callable, so I need the same kind of stop-in-the-middle hack to return the symbol of a function initialiser, plus an additional hack to look for and add call signatures when resolving members of the export *
.
So I reverted it. Can you come up with a way to improve the current hack? I think one-level resolution is fine even if not; commonjs usage is ossified and I think the support can be accordingly restrictive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I reverted it. Can you come up with a way to improve the current hack?
Honestly? 1cffb6e but go a little bit farther - with slightly more accompanying checker complexity - rather than tossing it into the export*
symbol table member and fishing it out, make a new special symbol table member for cjs export assignments that does the whole "copy the whole target and add the other exports on, too" rigamarole on export table lookup, complete with custom conflict resolution logic. Unfortunately, it definitely doesn't behave like any other AST construct we have at present because it's an export=
and a export*
symbol all rolled into one (or perhaps a mutable export=
).
I think a lot of why the strada implementation feels jank isn't because of the weird expression binding, but rather is because of our refusal to handle it like it's its own thing and instead patching its' behaviors on top of the existing ones. I'd rather handle it explicitly as its' own thing through the whole process and lower my worries that we're either shoehorning it into something it's not quite equivalent to and breaking a bunch of legacy code (which is all this is here for! "new jsdoc code" is almost an oxymoron) or patching exceptions on in an ad-hoc way like we did in strada.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the other option is to loosen how we handle export=
internally to behave like the cjs module.exports=
in strada - then it could be equivalent to that, I guess. There's honestly very little reason to disallow
export = class Foo {}
export const a = 1
in cjs-targeting typescript other than historic strictness and taste (since it transpiles to the same declarations we're checking on the input side here).
I'm adding this to show the changes, but I plan to revert it. The results are more correct, but the amount of code in the checker is twice the previous commit, so I don't think the improvement is worthwhile.
This reverts commit 1cffb6e.
With this PR, top-level typedefs in file with commonjs exports (
module.exports=x
orexports=x
) are no longer added as exports of the file. They're added as exports of whatever isexport=
. I'm not sure that I declared the local and export='d symbols in the right way, so I'd appreciate expert opinions there.The big change compared to Strada is that only top-level typedefs are exported. But it never made sense for these two scoped type aliases to be exported, let alone both:
It's especially weird that a scoped alias would be inaccessible at the top-level, but then accessible in a different file.